Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fnv/core verifier #939

Merged
merged 77 commits into from
Aug 16, 2023
Merged

fnv/core verifier #939

merged 77 commits into from
Aug 16, 2023

Conversation

curiecrypt
Copy link
Collaborator

@curiecrypt curiecrypt commented May 25, 2023

Content

A full node verifier feature that does not require checking signers' identities externally.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)

Comments

The idea is to provide a core verifier (previously full-node verifier) that has

  • A list of eligible parties,
  • Total stake in the system,

and be able to verify the signatures that are generated without the registration information, i.e., avk.
In this case, the signer who generates this signature has all the fields of StmSigner except from closed_reg.
So, closed_reg field is updated as Option. If the signer wants to participate in Mithril certificate generation, then it must have the closed_reg and this signer is generated with the function StmInitializer::new_signer(). Otherwise, the signer can only generate signatures to be validated by a core verifier. We call this signer core_signer, and it is created with the function StmInitializer::new_core_signer(). Note that in this version the core signer is built upon the StmInitializer, but it does not need to be built upon the StmInitializer depending on the functionality it provides.

StmSigner now provides two signing functions, one for a core signature and one for a signature to participate in certificate generation.
A core signature is generated by StmSigner::core_sign() function without closed registration and can be verified by the core verifier. This function simply creates a signature for a given message. StmSigner::sign() function calls core_sign() with msgp = (Commitment || msg), and it produces a signature for the Mithril certificate.

Similarly, StmSig is updated to provide

  • core_verify: Verifies sigma and checks the indices. This function verifies the StmSigs to be
    verified by full node verifier.
  • verify: Runs core_verify with msgp. This function is responsible for verifying StmSigs for mithril functionality.

A new struct StmSigRegParty including StmSig and RegParty is created. Consequently, the field signatures of StmAggrSig is now of the form Vec<StmSigRegParty> instead of Vec<(StmSig, RegParty)>. This change is done to avoid duplicate codes since the core verifier and StmAggrSig have many common operations.

A struct CoreVerifier containing eligible_parties and total_stake is created. Core verifier implements the following:

  • setup: Collects unique signers in eligible_parties, calculates the total_stake, and returns the CoreVerifier.
  • dedup_sigs_for_indices: This function is adopted from StmClerk::dedup_sigs_for_indices, and the new function can be used by both StmClerk and CoreVerifier.
  • preliminary_verify: Verify all signatures whether they all won the lottery, if the indices are unique, and if the quorum is achieved.
  • collect_sigs_vks: Collect and return the signatures and verification keys for given lists of StmSigRegParty.
  • verify: Maps the signatures with the eligible parties by map_sig_party. Runs dedup_sigs_for_indices to get unique signatures. After running preliminary_verify, it collects sigs and vks with collect_sigs_vks and verifies the aggregate.

The functions of CoreVerifier is designed to be used by StmAggrSig since they have similar operations.

StmAggrSig::preliminary_verify verifies all checks from signatures except for the signature verification itself.
Indices and quorum are checked by CoreVerifier::preliminary_verify with msgp.
It collects leaves from signatures and checks the batch proof. After the batch proof is checked, it collects and returns the signatures and verification keys to be used by aggregate verification.

Minor changes:

Error enums are updated for new functionality. to_bytes and from_bytes functions are implemented for StmSigRegParty, and they are used in StmAggrSig. Unused functions StmSigner::get_closed_reg() and StmSigner::compute_avk() are removed.

ToDo

  • Implement module test for CoreVerifier.
  • Remove StmClerk::dedup_sigs_for_indices() and its related tests.
  • Implement proptest for CoreVerifier::dedup_sigs_for_indices().
  • Implement full protocol test for CoreVerifier.
  • Implement benches for CoreVerifier.
  • Check the compatibility of the rest of the project with new features.
  • Update Mithril change log.
  • Update Mithril-stm README.
  • Update PR description.

Issue(s)

Closes #839

mithril-stm/benches/stm.rs Fixed Show fixed Hide fixed
mithril-stm/benches/stm.rs Fixed Show fixed Hide fixed
@github-actions
Copy link

github-actions bot commented May 25, 2023

Test Results

    3 files  ±0    17 suites  +1   7m 5s ⏱️ +19s
658 tests +2  658 ✔️ +2  0 💤 ±0  0 ±0 
698 runs  +2  698 ✔️ +2  0 💤 ±0  0 ±0 

Results for commit 22972f7. ± Comparison against base commit 09dc5e6.

This pull request removes 2 and adds 4 tests. Note that renamed tests count towards both.
mithril-stm::test_stm_protocol ‑ test_full_protocol
mithril-stm::test_stm_protocol ‑ test_full_protocol_batch_verify
mithril-stm ‑ stm::tests::test_core_verifier
mithril-stm::stm_core ‑ test_core_verifier
mithril-stm::stm_protocol ‑ test_full_protocol
mithril-stm::stm_protocol ‑ test_full_protocol_batch_verify

♻️ This comment has been updated with latest results.

@curiecrypt curiecrypt marked this pull request as ready for review May 25, 2023 15:41
@curiecrypt curiecrypt temporarily deployed to testing-preview May 25, 2023 15:46 — with GitHub Actions Inactive
Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good overall. Left some comments. However, I'm not convinced in the naming. verify_avk could be interpreted as verifying the aggregate key. Maybe instead of verify and verify_avk, we could call core_verify and verify. And both functions should have good documentation explaining their differences.

mithril-stm/src/ChangeLog_FNV.md Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
@curiecrypt curiecrypt temporarily deployed to testing-preview May 29, 2023 12:29 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview May 29, 2023 14:30 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview May 29, 2023 15:01 — with GitHub Actions Inactive
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made another pass. Still haven't looked at the tests. I would also make the suggestion of not using SignerAvk, as it does not seem clear. I would probably distinguish the two signers (and any other two structures) with the core keyword. SignerCore for the one that does not need the merkle tree, and Signer for the one that needs it.

Will go over the tests once the comments are addressed. Good work!

mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
@curiecrypt curiecrypt temporarily deployed to testing-preview June 6, 2023 17:45 — with GitHub Actions Inactive
@curiecrypt curiecrypt marked this pull request as draft June 6, 2023 17:47
@curiecrypt curiecrypt temporarily deployed to testing-preview June 7, 2023 13:02 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview June 7, 2023 15:44 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview June 8, 2023 10:52 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview June 8, 2023 15:39 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview June 8, 2023 16:12 — with GitHub Actions Inactive
@curiecrypt curiecrypt marked this pull request as ready for review June 12, 2023 11:24
@curiecrypt curiecrypt marked this pull request as draft June 12, 2023 11:36
@curiecrypt curiecrypt marked this pull request as ready for review June 12, 2023 11:37
@curiecrypt curiecrypt marked this pull request as draft June 13, 2023 13:23
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
@curiecrypt curiecrypt temporarily deployed to testing-preview June 14, 2023 16:33 — with GitHub Actions Inactive
@curiecrypt curiecrypt marked this pull request as ready for review June 14, 2023 17:05
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
mithril-stm/src/stm.rs Fixed Show fixed Hide fixed
@curiecrypt curiecrypt temporarily deployed to testing-preview June 15, 2023 13:02 — with GitHub Actions Inactive
@curiecrypt curiecrypt temporarily deployed to testing-preview August 15, 2023 13:01 — with GitHub Actions Inactive
Copy link
Contributor

@iquerejeta iquerejeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the tests passing, this implies that the serialisation is backwards compatible. It looks good to me 👍

@curiecrypt curiecrypt merged commit bbb995c into main Aug 16, 2023
25 of 33 checks passed
@curiecrypt curiecrypt deleted the fnv/dependent-signer-struct branch August 16, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full node verifier
2 participants